lt-body: add enableScaffolding option #421

buschtoens · 2017-06-08T07:57:15Z

This PR adds a scaffolding row (just like with {{lt-head}}) to the {{lt-body}} component.

This way it is possible to have a first row with colspans.

The scaffolding row can optionally be enabled like so:

{{t.body enableScaffolding=true}}

Also removes the now unnecessary style attribute binding from cells/base, which in theory should yield a small performance boost. 🚀

buschtoens · 2017-06-08T10:21:30Z

Tests failing... I'll look into that.

buschtoens · 2017-06-08T11:07:15Z

The tests for {{lt-body}} are now aware of .lt-scaffolding-row and ignore these.

alexander-alvarez · 2017-06-08T13:57:04Z

addon/components/cells/base.js

  classNameBindings: ['align', 'isSorted', 'column.cellClassNames'],

  isSorted: computed.readOnly('column.sorted'),

-  style: computed('column.width', function() {


Are we concerned that this no longer happens when enableScaffolding is set to false?

I would argue no. Currently, these style attributes don't really have an effect anyway, as the table-layout: fixed makes all but the first row ignore custom widths.

As long as the user doesn't overwrite the table-layout: fixed rule, nothing should change.

The only case where this could matter is, when the user actively decides to change the display from table to something like flex.

I can undo this, or make it depend on whether enableScaffolding is true or false.

buschtoens · 2017-06-12T08:33:56Z

addon/components/lt-body.js

+   * @type {Boolean}
+   * @default true
+   */
+  enableScaffolding: true,


Maybe it would be more sensible to make this a CP that's the inverse of overwrite.

As we're prepending a scaffolding row, this could lead to some unexpected behavior for people that use selectors like :first-child.

buschtoens · 2017-06-12T08:35:04Z

@offirgolan Would you like to chime in on the issues raised above? I'd like move this PR forward. :)

offirgolan · 2017-06-16T02:51:28Z

@buschtoens this is a risky change as it could introduce many unanticipated issues.

Can you give me an example as to how you would create a table with the first row having colspans? Maybe we can tackle this in a different way.

buschtoens · 2017-06-16T10:40:58Z

I needed to implement headings for table sub sections; basically a slimmed down version of the headings in #434 (sticky rows).

I tested my branch in our project and couldn't find any problems, but i can understand the hesitation and kinda feel the same. Maybe we could inverse the enableScaffolding so users explicitly have to opt-in? Just like with enableSync.

Alternatively, I'm sure I'll be able to come up with some position: absolute magic to achieve faux colspan.

buschtoens · 2017-06-22T15:59:59Z

I made enableScaffolding default to false and re-added the style binding on BaseCell. This way, if users don't explicitly opt-in, nothing changes at all. I also fixed #404 while I was at it.

When enableScaffolding=true the style CP short circuits and always returns null. This way we save some unnecessary DOM updates.

alexander-alvarez · 2017-06-22T18:45:41Z

tests/integration/components/lt-body-test.js

@@ -33,7 +33,7 @@ test('row selection - enable or disable', function(assert) {

  this.render(hbs `{{lt-body table=table sharedOptions=sharedOptions canSelect=canSelect}}`);

-  let row = this.$('tr:first');
+  let row = this.$('tr:not(.lt-scaffolding-row):first');


out of curiousity did you need to update these tests? enableScaffolding isn't set

In the original state of this PR with enableScaffolding defaulting to true, this was necessary. It isn't anymore, but I precautiously still included this commit. This way the tests will continue to work, even if we might decide to set enableScaffolding by default in the future, or when we copy and paste these cases to write some new enableScaffolding specific tests.

If you feel that this makes the tests harder to comprehend / maintain, I have absolutely no objections to cancel out this commit. 🙂

Got it. That makes sense.

It's a little confusing since it isn't set.

As an aside there aren't any actual tests that verify enableScaffoliding puts a scaffolding row in the DOM 😲 🚨 🙄

That is... correct. 😨

I could swear, that I've written these. 🤔
Gonna push the tomorrow from the office. ☺️

@buschtoens I dont think this is needed since you have specific tests for when it is enabled. I prefer it like that since it is more contained. If we enabled it by default in the future, we can just turn it off for these test cases.

buschtoens · 2017-06-23T11:50:38Z

I added a test case asserting, that:

the first row of the <tbody> is a scaffolding row
the second row of the <tbody> is not a scaffolding row
the second row of the <tbody> has no style attribute
the first actual data row has the correct widths

alexander-alvarez · 2017-06-23T13:58:56Z

tests/integration/components/lt-body-test.js

+    scaffoldingRow.hasClass('lt-scaffolding-row'),
+    'the first row of the <tbody> is a scaffolding row'
+  );
+  assert.ok(


assert.notOk

Actually I was gonna use that, but I wanted to stay in line with the other tests in this file, which all use

assert.ok(!condition);

Should I leave it or still change to notOk?

lol I didn't even realize.
leave it for now... I'll do a PR to migrate to using ember-native-dom-helpers in the tests and I'll fix it up there

buschtoens · 2017-06-25T02:55:55Z

@offirgolan Any objections? enableScaffolding now defaults to false, so users have to opt-in explicitly. There also are sufficient test cases asserting, that the actual data rows have the specified widths.

Unless you don't want this feature in ELT, I'll merge. 😉

offirgolan · 2017-06-27T06:31:37Z

addon/components/cells/base.js

+    let column = this.get('column');
+    if (!column) {
+      return null;
+    }


let column = this.get('column'); if (this.get('enableScaffolding') || !column) { return ''; }

offirgolan · 2017-06-27T06:33:28Z

tests/integration/components/lt-body-test.js

@@ -33,7 +33,7 @@ test('row selection - enable or disable', function(assert) {

  this.render(hbs `{{lt-body table=table sharedOptions=sharedOptions canSelect=canSelect}}`);

-  let row = this.$('tr:first');
+  let row = this.$('tr:not(.lt-scaffolding-row):first');


@buschtoens I dont think this is needed since you have specific tests for when it is enabled. I prefer it like that since it is more contained. If we enabled it by default in the future, we can just turn it off for these test cases.

Also removes the style attribute binding from cells/base, if enabled. Also fixes adopted-ember-addons#404.

buschtoens · 2017-06-27T07:26:51Z

Thanks for reviewing guys. I made the requested changes. Tests are passing as well.

buschtoens changed the title ~~lt-body: add scaffoling row by default~~ lt-body: add scaffolding row by default Jun 8, 2017

alexander-alvarez reviewed Jun 8, 2017

View reviewed changes

buschtoens commented Jun 12, 2017

View reviewed changes

buschtoens force-pushed the body-scaffolding branch from fa85616 to 9ff9d9a Compare June 22, 2017 15:56

buschtoens requested review from offirgolan and alexander-alvarez June 22, 2017 16:00

alexander-alvarez reviewed Jun 22, 2017

View reviewed changes

alexander-alvarez reviewed Jun 23, 2017

View reviewed changes

buschtoens changed the title ~~lt-body: add scaffolding row by default~~ lt-body: add enableScaffolding option Jun 23, 2017

offirgolan requested changes Jun 27, 2017

View reviewed changes

buschtoens added 2 commits June 27, 2017 09:09

lt-body: add enableScaffolding option

6cb7992

Also removes the style attribute binding from cells/base, if enabled. Also fixes adopted-ember-addons#404.

tests/lt-body: add tests for scaffolding

d79382f

buschtoens force-pushed the body-scaffolding branch from ef74e49 to d79382f Compare June 27, 2017 07:09

buschtoens merged commit f6f2334 into adopted-ember-addons:master Jun 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lt-body: add enableScaffolding option #421

lt-body: add enableScaffolding option #421

buschtoens commented Jun 8, 2017 •

edited

Loading

buschtoens commented Jun 8, 2017

buschtoens commented Jun 8, 2017

alexander-alvarez Jun 8, 2017

buschtoens Jun 8, 2017

buschtoens Jun 12, 2017

buschtoens Jun 12, 2017

buschtoens commented Jun 12, 2017

offirgolan commented Jun 16, 2017

buschtoens commented Jun 16, 2017 •

edited

Loading

buschtoens commented Jun 22, 2017

alexander-alvarez Jun 22, 2017

buschtoens Jun 22, 2017 •

edited

Loading

alexander-alvarez Jun 22, 2017

buschtoens Jun 22, 2017

offirgolan Jun 27, 2017

buschtoens commented Jun 23, 2017 •

edited

Loading

alexander-alvarez Jun 23, 2017

buschtoens Jun 23, 2017

alexander-alvarez Jun 23, 2017

buschtoens commented Jun 25, 2017 •

edited

Loading

offirgolan Jun 27, 2017

offirgolan Jun 27, 2017

buschtoens commented Jun 27, 2017

lt-body: add enableScaffolding option #421

lt-body: add enableScaffolding option #421

Conversation

buschtoens commented Jun 8, 2017 • edited Loading

buschtoens commented Jun 8, 2017

buschtoens commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Jun 12, 2017

offirgolan commented Jun 16, 2017

buschtoens commented Jun 16, 2017 • edited Loading

buschtoens commented Jun 22, 2017

Choose a reason for hiding this comment

buschtoens Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buschtoens commented Jun 27, 2017

buschtoens commented Jun 8, 2017 •

edited

Loading

buschtoens commented Jun 16, 2017 •

edited

Loading

buschtoens Jun 22, 2017 •

edited

Loading

buschtoens commented Jun 23, 2017 •

edited

Loading

buschtoens commented Jun 25, 2017 •

edited

Loading